Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: AND/OR query filters #3934

Closed
wants to merge 10 commits into from
Closed

feat: AND/OR query filters #3934

wants to merge 10 commits into from

Conversation

saihaj
Copy link
Member

@saihaj saihaj commented Sep 14, 2022 •
edited
Loading

Enables users to execute queries with logical operators.

Query Example:

{
  musicians(where: {OR: {name: "John", id: "m2"}}) {
    name
    id
  }
}

Question: should we limit this to only one level deep?

Closes #579

@saihaj
graphql: add AND/OR filter
0532a1a
@saihaj
store: parse and resolve AND/OR filter
b09b977 
some more things

test case

no log

cleanup

more tests
lutter
lutter requested changes Sep 15, 2022
View reviewed changes
Copy link
Collaborator

@lutter lutter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Looks good in general, but there's some test failures.

One question: are these filters case sensitive? Seeing AND when everything else is lower case feels a little strange.

@saihaj
Copy link
Member Author

saihaj commented Sep 15, 2022

Seeing AND when everything else is lower case feels a little strange.

In the ticket and to make it more differentiable made these logical filters uppercase. Don’t have a preference so happy to do whatever you think is the best.

@azf20
Copy link
Contributor

azf20 commented Sep 15, 2022

Agree with David, I think lowercase would be more in keeping. And nice work!

Can you clarify what you mean by The current implementation would resolve OR only one level deep but AND is recursively constructed. - so you could have X OR (Y AND Z), and you could have X AND (Y OR Z) but not X OR (Y OR Z)?

@saihaj

This comment was marked as outdated.

Sign in to view
@saihaj
Copy link
Member Author

saihaj commented Sep 15, 2022

so you could have X OR (Y AND Z), and you could have X AND (Y OR Z) but not X OR (Y OR Z)?

@azf20 updated it so it no longer is limiting. User can make as much recursively deep AND/OR filter they if they chose to do so.

there's some test failures.

@lutter fixed the schema tests

@saihaj saihaj requested a review from lutter September 15, 2022 16:14
saihaj
saihaj commented Sep 16, 2022
View reviewed changes
graphql/src/schema/api.rs Outdated Show resolved Hide resolved
saihaj
saihaj commented Sep 16, 2022
View reviewed changes
graphql/src/schema/ast.rs Outdated Show resolved Hide resolved
@dotansimha
Copy link
Contributor

dotansimha commented Sep 19, 2022 •
edited
Loading

@saihaj i think we need a rebase here, and then we are good to go in terms of code review?

@lutter
Copy link
Collaborator

lutter commented Sep 22, 2022

Quick note on rebasing: please always rebase with git rebase master <mybranch> not by merging master into the branch since that leads to funky looking merge commits.

lutter
lutter approved these changes Oct 4, 2022
View reviewed changes
Copy link
Collaborator

@lutter lutter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Sorry it took so long to review.

Just add another test like I suggested in the comment (or more if you can think of more ;) ) and then feel free to rebase and commit.

graphql/tests/query.rs Show resolved Hide resolved
@saihaj saihaj marked this pull request as draft October 10, 2022 10:15
@saihaj saihaj marked this pull request as ready for review October 11, 2022 14:30
lutter
lutter reviewed Oct 12, 2022
View reviewed changes
graphql/tests/query.rs
musicians(
where: {
or: {and: {name: \"John\", id: \"m1\" },
or: {and: {name: \"Lisa\", id: \"m2\" }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought repeating the same key here was prohibited by the GraphQL spec, or am I misinterpreting this?

Also cc @azf20 for the syntax of writing (name = 'John' and id = 'm1') or (name = 'Lisa' and id = 'm2') As an alternative, we could maybe have and and or take either an object (and: {name: "John", id: "m1"}) or an array of objects (or: [{and: {name: "John", id: "m1"}}, {and: {name: "Lisa", id: "m2"}}])

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s the way I formatted the query for test it may look like prohibited syntax but it isn’t :)

query {
  musicians(
    where: {
      or: {
        and: { name: "John", id: "m1" }
        or: { and: { name: "Lisa", id: "m2" } }
      }
    }
  ) {
    id
    name
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saihaj I am a bit confused on what that query is doing, is it effectively:

(name == "John" AND id == "m1") OR (name == "Lisa" AND id == "m2")

Or is it something different? If so I think that syntax is a bit confusing

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes @azf20 that is what the query is doing

@dotansimha dotansimha requested a review from lutter October 16, 2022 02:47
@azf20
Copy link
Contributor

azf20 commented Oct 24, 2022

If we proceed with #4080 should we close this @saihaj ?

@dotansimha dotansimha closed this Oct 25, 2022
@saihaj saihaj deleted the saihaj/579 branch November 10, 2022 02:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dotansimha dotansimha dotansimha approved these changes

@tilacog tilacog Awaiting requested review from tilacog

@kamilkisiela kamilkisiela Awaiting requested review from kamilkisiela

@azf20 azf20 Awaiting requested review from azf20

@leoyvens leoyvens Awaiting requested review from leoyvens

@lutter lutter Awaiting requested review from lutter

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Expose AND and OR logical operators in GraphQL query filters
5 participants
@saihaj @azf20 @dotansimha @lutter @kamilkisiela